When used in the allowListGuard case, I don't think we should consider contains, indexOf and lastIndexOf as sufficient sanitizers, as they do not provide sufficient protection for absolute paths provided by the user (e.g. https://cwe.mitre.org/data/definitions/36.html).

Consider this example:

Path path = Paths.get("/etc/passwd");
if (!path.contains("..")) {
  if (path.contains("mySpecialCase")) {
    sink(path);
  }
}

This is considered an allow list sanitizer, but is clearly not sufficient for absolute paths.

For regionMatches, I think we should only allow it when it is a prefix (i.e. starting at index zero). We may also want to do some basic analysis of matches to consider if it starts with .* or similar, which would, again, allow absolute paths through.

I think these non-prefix cases are also problematic in the disallowListGuard case as well, because we don't consider what is being disallowed. In order to be an effective sanitizer I think we need to a least consider whether the standard path separators such as / and \ are being disallowed.

These are good suggestions, thanks! I'm actually modifying the logic of this sanitizer even further, so I'll try to fit these ideas in, too.

Keep in mind that, at some point, we need to compromise. Overfitting to one case may leave others out (either introducing FNs or FPs). For instance, in your example:

Path path = Paths.get("/etc/passwd");
if (!path.contains("..")) {
  if (path.contains("mySpecialCase")) {
    sink(path);
  }
}

The payload would not actually reach the sink because /etc/passwd doesn't contain mySpecialCase. I agree the payload would go through if it looked like /anotherDirectory/mySpecialCase/*, whereas the intention of the developer probably was trying to restrict the path to /mySpecialCase/*, but the attacker's options are way more limited in this case (it funnily starts to look like a potential partial path traversal instead).

Just saying this because some times contains et al might be enough to avoid exploitation — so not considering them would introduce FPs, but adding them could introduce FNs in other specific scenarios.

Sorry, I chose a bad example for the allowListGuard, should have been something like this:

String path = "/etc/passwd";
if (!path.contains("..")) {
  if (path.contains("pass")) {
    sink(path);
  }
}

I agree the payload would go through if it looked like /anotherDirectory/mySpecialCase/, whereas the intention of the developer probably was trying to restrict the path to /mySpecialCase/, but the attacker's options are way more limited in this case (it funnily starts to look like a potential partial path traversal instead).

Although the contains may limit the attackers choice of full path, it doesn't prevent an attacker from creating a path with any directory prefix on the system[1]. That always seems undesirable and unintended and although it may be more difficult to exploit that a vanilla tainted path example, it's also hard to convince yourself that it's not exploitable because it depends on paths accessible to the process, software installed on the system and the exact set of allowed patterns.

Out of interest, do we have some examples of this type of allow list sanitizer?

Keep in mind that, at some point, we need to compromise. Overfitting to one case may leave others out (either introducing FNs or FPs).

fwiw, for this rule I think poor sanitizers that cause false negatives are more of a problem than good sanitizers that cause false positives.

[1]: Assuming the value used as a path is created in a way that's vulnerable to absolute path traversal e.g. new File(taintedValue); and not new File(parent, taintedValue). In an ideal world I think we would actually separate absolute and relative path traversal with flow states, and only consider the latter for relative path traversal, but I think that's probably a separate issue. 🙂

fwiw, for this rule I think poor sanitizers that cause false negatives are more of a problem than good sanitizers that cause false positives.

You have more context from users, so let's follow your approach. See 60b31c6: AllowListGuard now only considers prefix checks (and not partial matching checks), and regionMatches and matches apply heuristics to make sure a prefix is being checked.

See the other thread below for the analogous changes in BlockListGuard.

In an ideal world I think we would actually separate absolute and relative path traversal with flow states, and only consider the latter for relative path traversal, but I think that's probably a separate issue. 🙂

This sounds like a good improvement to do in a future PR. Noted, thanks!

For the blockListGuard we require that the user has mitigated for URL decoding issues. Is this really a universal requirement to use a block list?

As far as I know, the only impact URI decoding has on tainted path issues is that decoding should not occur after path checks, because the path may only be malicious after decoding.

However, requiring the user decode the path before checking can be outright harmful if using a library or framework that already URI decodes remote flow sources, or for remote flow sources that don't require URI decoding.

In general, block lists are less than ideal sanitizers, and allow lists should be preferred. One of the reasons for that is that it's easy to forget about a bad case, or to not consider different encodings when checking the externally-provided data.

So this condition is there to force block lists approaches to be a little more robust if we want them to be considered valid sanitizers. I agree that the requirement of URL decoding prior to the check might be too much, but theoretically your examples:

if using a library or framework that already URI decodes remote flow sources, or for remote flow sources that don't require URI decoding.

could be fixed by marking those specific remote flow sources as UrlDecoderSanitizers themselves.

The alternative would be that a simple payload.contains("..") would be considered a sanitizer, and then users would complain as well, since we all know that that doesn't work at all. Again, at some point we need to trade FNs for FPs, or vice versa.

The alternative would be that a simple payload.contains("..") would be considered a sanitizer

I agree that we don't want that, but I don't think requiring a URI decoding step beforehand makes payload.contains("..") a sufficient sanitizer for absolute path traversal, for the same reasons as above.

could be fixed by marking those specific remote flow sources as UrlDecoderSanitizers themselves.

I don't think requiring URI decoding before the check prevents any security issues. The URI decoding issue is only a problem if you URI decode between the check and the access, e.g.

x = decode(source())
if (check(x)) {
  new File(x); // fine
}

x = source()
if (check(x)) {
  new File(x); // also fine, File won't URI decode
}

x = source()
if (check(x)) {
  new File(decode(x)); // bad!
}

For example, in the Spring vulnerability the issue wasn't that they forgot to URI decode before their tainted path check. Instead, the problem was that they ran second decode step after the check, but before the value was used.

Clearly, requiring taking double-encoding into account only makes sense if the sink itself implicitly URL-decodes the value, which happens in some dispatch/forward/redirect vulnerabilities. I think this could actually be another interesting use case for flow states (see the comment I added in the now FN test in https://github.com/github/codeql/pull/10177/files#diff-6bc44c5765a6e460db430d2c13a69b22afe818b46bc96d630508b5e1d1cd6eb1R103-R106).

So, with that in mind, I followed your advice and removed the URL-decode requirement for blockListGuard. Instead, I added the same canonicalization/path traversal check requirement that allowListGuard has, assuming that the block list will check for dangerous prefixes (like /data in Android). The only case that would not need normalization would be contains(File.seperator) (or contains("/") or contains("\\")), but to special-case those bad words we would need to create yet another guard. We can discuss if you think that'd be worth it.